Skip to content

fix(parser): Reject a table version instead of ignoring it - #1827

Open
PingLiuPing wants to merge 1 commit into
facebookincubator:mainfrom
PingLiuPing:lp_time_travel_sql
Open

fix(parser): Reject a table version instead of ignoring it#1827
PingLiuPing wants to merge 1 commit into
facebookincubator:mainfrom
PingLiuPing:lp_time_travel_sql

Conversation

@PingLiuPing

@PingLiuPing PingLiuPing commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

SELECT ... FROM t FOR VERSION AS OF 8 parsed and then read the current snapshot. The grammar accepts the FOR ... AS OF/BEFORE clause and the AST carries a slot for it, but visitTableName built the table without ever reading tableVersionExpression, so the version was dropped and the query silently answered from the wrong snapshot.

The parser now rejects it with "Table version (time travel) is not supported yet". Every form the grammar accepts is rejected the same way, all four version keywords, in both the AS OF and BEFORE forms. So a versioned reference cannot silently read the current snapshot.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Sep 4, 2026
@PingLiuPing PingLiuPing mentioned this pull request Sep 4, 2026
4 tasks
@PingLiuPing

Copy link
Copy Markdown
Contributor Author

@mbasmanova Could you help take a look at this PR? Thank you.

@mbasmanova mbasmanova left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for picking this up — a version that parses and is then silently dropped is the worst of the options.

Could this land as parse-and-reject only? No connector overrides supportsTableTimeTravel(), so FOR VERSION AS OF 8 never reaches PlanBuilder::tableScan, in a test or in a deployment. The snapshot on SchemaTableName, its place in hashing and identity, the serde field, the PlanBuilder parameter and the connector hook are all unexercised.

Unexercised plumbing does not stay right while it waits. The findView call that follows findTable in RelationPlanner::processTable gets the snapshot-bearing name, and findView is a views_.find(tableName), so the first connector to opt in would see "table not found" for a versioned view rather than a message about views.

The rejection is the valuable half of this change and stands on its own. When a connector is ready to honor a snapshot, the shape can be settled with a real caller in hand — and Presto's is worth a look first: it keeps SchemaTableName a name and passes ConnectorTableVersion beside it into getTableHandle, with the resolved version living in the table handle. That type also carries a version operator for AS OF versus BEFORE and a typed value rather than an integer, since FOR TIMESTAMP AS OF is a timestamp — so an optional<int64_t> on the name would have to be replaced rather than extended once either rejected form is implemented.

Also, rejectsUnsupportedTableVersionForms names the expectation rather than what it covers. tableVersion reads like its neighbours, tablesample and everything.

@PingLiuPing

Copy link
Copy Markdown
Contributor Author

@mbasmanova Thank you for the comments!

Dropped the plumbing commit, the PR now only reject table version alone. Every version keyword the grammar accepts is rejected with the same message.

On why the second commit was there: I have most of an Iceberg connector implemented locally, where snapshot pinning and a per snapshot table cache already work against a REST catalog. So I was writing the parser end against a consumer I could see. You're right that nothing in this repo exercises the plumbing in the meantime.

Thank you for the pointer to Presto's shape. ConnectorTableVersion passed beside the name into getTableHandle, with a version operator for AS OF versus BEFORE and a typed value rather than an integer, is better than an optional<int64_t> on SchemaTableName. I'll bring the plumbing back in that shape when the connector is ready to honor a snapshot.

@mbasmanova mbasmanova left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you — and thank you for saying where the plumbing came from. Writing the parser end against a connector you can see makes sense; it just could not be reviewed here without one.

The description still describes the change that was dropped: snapshot id travelling end to end, SchemaTableName carrying it as part of table identity, PlanBuilder::tableScan accepting it, connectors opting in through supportsTableTimeTravel(). None of that is in the diff now, and the description becomes the commit message. Could you rewrite it to what this does — reject every table version form the grammar accepts, so a versioned reference cannot silently read the current snapshot?

tableVersion repeats the same call four times with one expected message, and each statement repeats SELECT * FROM nation FOR. Looping over just the version clauses would leave only the part that varies.

@PingLiuPing PingLiuPing changed the title feat: Carry FOR VERSION AS OF from the parser into the plan fix(parser): Reject a table version instead of ignoring it Sep 10, 2026
@PingLiuPing

Copy link
Copy Markdown
Contributor Author

@mbasmanova Thank you. Fixed the testcase, and updated PR title and description.

@mbasmanova mbasmanova left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

The comment above tableVersion repeats the one in visitTableName almost word for word. The reasoning belongs at the code; here the test name and the expected message already say what it covers, and two copies will drift.

Also a typo for the commit message: "in both the AS OF and BEFORE forms" has the space inside the backticks.

@meta-codesync

meta-codesync Bot commented Sep 10, 2026

Copy link
Copy Markdown

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this in D119447682.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot. ready-to-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants